Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adding clrgc variant with regions enabled #89129

Merged
merged 3 commits into from
Jul 28, 2023
Merged

Conversation

mangod9
Copy link
Member

@mangod9 mangod9 commented Jul 18, 2023

we already ship standalone clrgc.dll with segments. This adds another clrgcr.dll with regions enabled.

@ghost
Copy link

ghost commented Jul 18, 2023

Tagging subscribers to this area: @dotnet/gc
See info in area-owners.md if you want to be subscribed.

Issue Details

we already ship standalone clrgc.dll with segments. This adds another clrgcr.dll with regions enabled.

Author: mangod9
Assignees: -
Labels:

area-GC-coreclr

Milestone: -

@@ -104,6 +104,16 @@ list(APPEND GC_SOURCES ${GC_HEADERS})

convert_to_absolute_path(GC_SOURCES ${GC_SOURCES})

# clrgcr is build with standalone+regions
if (CLR_CMAKE_TARGET_ARCH_ARM64 OR CLR_CMAKE_TARGET_ARCH_AMD64)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this builds clrgcr.dll (and libclrgcr.so) on 64 bit platforms. We could change naming if required.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: for the sake of readability, would it be better to change the name to: clrgc_regions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

didnt want to name it as regions since longer term it might be for a different feature. Perhaps something like experimental or something. Maybe rename it clrgce for experimental ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clrgcexp?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah that works, will rename.

@mangod9
Copy link
Member Author

mangod9 commented Jul 18, 2023

CI failures look unrelated but will take a closer look.

@mangod9
Copy link
Member Author

mangod9 commented Jul 19, 2023

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mangod9
Copy link
Member Author

mangod9 commented Jul 20, 2023

Ci is now green. Please review, we can discuss naming, but anything we come up with would most likely require changing.

Copy link
Member

@mrsharm mrsharm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! One small nit but it's not necessary.

@mangod9 mangod9 merged commit b869e8d into dotnet:main Jul 28, 2023
107 checks passed
@mangod9 mangod9 deleted the clrgc_regions branch July 28, 2023 05:50
markples pushed a commit to markples/runtime that referenced this pull request Aug 23, 2023
* adding clrgc variant with regions enabled

* only build regions for 64 bit platforms

* rename to clrgcexp for experimental
@ghost ghost locked as resolved and limited conversation to collaborators Aug 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants